Add operations to return WCS coordinates from argmin/argmax #680
Conversation
e-koch
changed the title
|
Where do we stand on this? You need review? Rebase? |
|
Rebase. Will do by the end of the day |
e-koch
force-pushed
the
add_wcs_argmin_max_operations
branch
from
1448c8e
to
aa8d15d
Compare
Codecov Report
@@ Coverage Diff @@
## master #680 +/- ##
==========================================
+ Coverage 80.19% 80.34% +0.14%
==========================================
Files 24 24
Lines 5505 5552 +47
==========================================
+ Hits 4415 4461 +46
- Misses 1090 1091 +1
Continue to review full report at Codecov.
|
|
@astrofrog @keflavich This is ready! |
There was a problem hiding this comment.
I like this but I wonder if we should return all world coordinates - there are cases where the RA/Dec may not line up with the pixel axes, for example they may be rotated by 45 deg. So then if you called argmax_world(axis=1) it's not clear if what would be returned is RA or Dec. Picking the world coordinate with the same index as the pixel axis only make sense in the cases where pixel and world axes are aligned?
|
@astrofrog Is this presently going to return incorrect results in the case of a rotated WCS? If so, we should special-case this for aligned axes only (we have that ability in generalized WCS, right?), and punt the general case for now. I'm not sure how much use there is for the general case anyway. |
|
@astrofrog @keflavich Here's the check I added for correlated image -> WCS axes: https://github.com/radio-astro-tools/spectral-cube/pull/680/files#diff-5efbc7ba09eb93125b163d09f8107e93a24aac38ed8791ef0b17b5ffb2fbea18R486 My understanding of I'm wondering if the changes in this PR should be restricted to only the spectral axis. I'm now not sure that any other case is well defined |
|
Current failures will be fixed in #688 |
…xis=None cases that aren't defined for these operations
…ns to make 2D maps (max/min, argmin/max)
…world cannot handle
e-koch
force-pushed
the
add_wcs_argmin_max_operations
branch
from
b06c187
to
eb56512
Compare
|
@astrofrog Can you have a quick look through the new changes? I've added checks for correlated pixel -> WCS axes. This otherwise ready to go, IMO |
There was a problem hiding this comment.
lgtm, though I have suggestions on the docstring.
also, there's a lot of duplicated code between argmax/argmin. Could we combine those into one with method='max'/method='min' and just pick between them, then have argmin_world just return argminmax_world(method='max', **kwargs) ?
Co-authored-by: Adam Ginsburg <[email protected]>
Co-authored-by: Adam Ginsburg <[email protected]>
|
@keflavich -- Thanks for the suggestion. I've added I think this is ready to merge if these last tests pass. |
Revised version in #665 that does not depend on #656
Adds
SpectralCube.argmin_worldandSpectralCube.argmax_worldto return the WCS coordinates of the argmin/max along a dimension. I've generalized a snippet from @low-sky for this.The main use is creating "peak velocity" maps from a cube (i.e., velocity at peak intensity):